feat: reimplement validator rewarder #69
Conversation
| @@ -32,28 +32,22 @@ contract ValidatorRewarder is IValidatorRewarder, UUPSUpgradeable, OwnableUpgrad | |||
| /// @notice The token that this rewarder mints | |||
| Recall public token; | |||
|
|
|||
| /// @notice The latest checkpoint height that rewards can be claimed for | |||
| /// @dev Using uint64 to match Filecoin's epoch height type and save gas when interacting with the network | |||
| uint64 public latestClaimedCheckpoint; | |||
There was a problem hiding this comment.
We don't need to store any state in this contract since we can dynamically compute rewards for each validator for every checkpoint. Reward calculation logic is also simplified
There was a problem hiding this comment.
Love the simplicity here @avichalp. So much red. Very good job on this, much simpler, and easier to maintain and also grok. I left a few comments and questions, but nothing too serious as this is a very directly implementation of the prior discussion. Good work!
| UD60x18 oneToken = ud(1 ether); | ||
|
|
||
| // Calculate (checkpointPeriod * 1 ether) / BLOCKS_PER_TOKEN using fixed-point math | ||
| return period.mul(oneToken).div(blocksPerToken).unwrap(); |
There was a problem hiding this comment.
This is great. Do we have a test to ensure that any overflow or "rounding" due to fixed point calcs is going to resolve to the correct total minted tokens over time? I.E. If we consistently do something like have each validator claim tokens every block, that we don't end up with more than 1 total token every 3 blocks?
There was a problem hiding this comment.
the FFI test tries to do that. It simulates the rewards for next 5 years and makes sure the that the actual minted amount is less than or equal to total minted tokens. There is some rounding (down) errors that accumulates over time. The test asserts that the error is bounded by: 0.0000000000001 tokens in 5 years
| expectedValidatorShare, | ||
| 1000, | ||
| 1000, // Allow for difference of up to 1000 base units |
|
This is looking good. Probably ready for another final round of review? Current on scope for the auditors as well. |
yep, waiting for @oed's review as well before merging |
|
We will do by commit hash. Let's merge this! |
Signed-off-by: avichalp <hi@avichalp.me>
Signed-off-by: avichalp <hi@avichalp.me>
Signed-off-by: avichalp <hi@avichalp.me>
avichalp
force-pushed
the
avichalp/rewarder-v2
branch
from
fe4d3b4 to
3e5299d
Compare
Signed-off-by: avichalp <hi@avichalp.me>
|
closes #74 |
This PR reimplements validator rewarder contract according to a fixed token increments. Please checkout the discussion here for more details about the current implementation: recallnet/ipc#151 (comment)
With these changes there won't be any "state" in the rewarder contract. The overall architecture remains the same. That is the validator rewarder is being called by the IPC when there is a new claim for rewards. We assume that IPC guarantees that there won't be any "double claiming". IPC notifies the validator rewarder with the checkpoint height, validator's address and the number of blocks that this validator has committed in the checkpoint.
Each checkpoint contains 600 blocks. Which means 200 new tokens (1 token per 3 blocks) are minted in each checkpoint. We just calculate the validator's share of these 200 new tokens i.e. the ratio of blocks that they have committed to total blocks in the checkpoint. We mint this amount to validator's address.